Skip to content

refactor: introduce EnvironmentIndex to make the served environment set runtime-mutable - #17

Open
gagantrivedi wants to merge 20 commits into
mainfrom
feat/environment-discovery
Open

refactor: introduce EnvironmentIndex to make the served environment set runtime-mutable#17
gagantrivedi wants to merge 20 commits into
mainfrom
feat/environment-discovery

Conversation

@gagantrivedi

@gagantrivedi gagantrivedi commented Aug 22, 2026

Copy link
Copy Markdown
Member

Thanks for submitting a PR! Please check the boxes below:

  • I have read the Contributing Guide.
  • I have added information to docs/ if required so people know about the feature.
  • I have filled in the "Changes" section below.
  • I have filled in the "How did you test this code" section below.

Changes

Contributes to Flagsmith/edge-proxy#128

Groundwork for environment auto-discovery: the served environment set becomes runtime-mutable. No behaviour change — static environment_key_pairs configs work exactly as today; discovery itself is a follow-up PR.

  • New EnvironmentIndex (src/environments.rs) replaces the startup-built key maps. Each environment is indexed under its client key and every server key; insert/remove return the replaced/removed entry so callers can invalidate caches.
  • One resolve_key() replaces the three unknown-key gates (ser. keys still 503 on /flags//identities as today — fixed in a follow-up).
  • remove_environment stops serving an environment and clears everything cached for it; the poll loop re-checks the index after each write so a removal completing mid-fetch can't pin stale data.
  • environment_key_pairs is now optional; a startup warning fires when nothing is configured.
  • Poll-failure logs now print the client key, not the ser. secret.

Accepted, deliberately: duplicated server keys are last-one-wins, and endpoint-response-cache residue on a racing removal is not guarded against (that cache is deprecated and disabled by default).

How did you test this code?

cargo test: 63 green, including index unit tests and tests/test_remove_environment.rs (removed environments are rejected, not served from primed caches, for both key kinds). No existing test expectations changed. Clippy and fmt clean.

Environments are now held in an EnvironmentRegistry that indexes each
record under its client key and every server key, replacing the
immutable key_mapping/server_to_client maps built at startup. Records
carry an EnvSource so statically configured environments can never be
evicted by future inventory reconciliation.

Adds EnvironmentsCache::remove_environment and
EnvironmentService::evict_environment so an environment can be
forgotten at runtime, including the endpoint caches that are consulted
before the key gate. Splits fetch_environment into a server-key
picker and a reusable fetch_document(server_key, if_modified_since).

No behaviour change for existing configs; the poll-failure log now
prints the client key instead of the server-side secret.
An omitted field now behaves like an explicitly empty list (which was
already accepted), so a future discovery-only config needs no static
pairs. Statically configured pairs keep working exactly as before.
main() now only loads settings and logging before delegating, so an
alternative binary can compose the proxy from the library crate.
The exhaustive literal broke compilation whenever AppSettings gained a
field; the other test files already spread from default().
The type is the domain concept of which environments the proxy serves,
not a service, so it moves out of services/ to src/environments.rs
beside cache/ — matching how comparable Rust proxies place such state
(e.g. unleash-edge's feature_cache). Drops the services re-export:
EnvironmentService is its only consumer.
Names the content: the key set of one environment (client key, server
keys, provenance). Also unabbreviates EnvSource to EnvironmentSource
and renames records() to snapshot() so no 'record' vocabulary is left.
Nothing reads it: an environment's provenance only matters to the
reconciliation that later phases introduce, and staticness is
derivable there from the immutable environment_key_pairs settings.
Reintroduce an explicit field if that derivation proves awkward.
Evict implies cache-pressure expulsion; this is a deliberate removal
from the served set, and the cache trait already calls its half
remove_environment, so the whole family now shares the verb.
A config that lists the same server key for two environments is a
misconfiguration we choose not to defend against; removal now just
drops the entry's keys unconditionally.
Endpoint-cache lookups run before the key gate, so an in-flight
request that passed the gate could write its result after
remove_environment's clears and have it served indefinitely; the poll
loop, iterating a pre-removal snapshot, could likewise re-insert a
removed environment's document and pin it until restart. Every cache
write now re-checks the index afterwards and clears what it wrote if
the key no longer resolves — remove_environment un-indexes before
clearing, so one of the two clears always runs last. Removal also
clears caches even when the key is unknown, so repeating it cleans any
residue.

The interleavings themselves aren't deterministically testable without
injection points; the tests pin each guard's behaviour instead.
Mirrors HashMap::insert and makes insert symmetric with remove: when a
later phase rotates keys via insert, the caller needs the dropped
server keys to invalidate request caches, which are keyed by presented
key and consulted before the key gate.
The duplicated-server-key failure shape, the /health-stays-red state a
key-less environment creates, and the actual reason run() lives in the
library were all design decisions living only in review threads.
A typo'd environment_key_pairs field name silently parses as an empty
set (serde ignores unknown fields), leaving a healthy-looking proxy
that rejects everything. Also pins the serde default with a test that
{} parses to an empty, valid config.
Nothing besides main.rs ever called it; the composition-binary story it
served is speculative, and it can return the day something real needs
it. The empty-config warning moves with the body.
The endpoint response cache is deprecated and disabled by default;
without it every request passes the key gate, so a cache write racing
a removal is not servable residue worth guarding against. The poll
re-insertion guard on the main environments cache stays — that cache
is load-bearing.
@gagantrivedi gagantrivedi changed the title refactor: make the served environment set runtime-mutable refactor: introduce EnvironmentIndex to make the served environment set runtime-mutable Aug 22, 2026
@gagantrivedi
gagantrivedi marked this pull request as ready for review August 22, 2026 09:56
@matthewelwell

Copy link
Copy Markdown

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The proxy now uses EnvironmentIndex to resolve client and server keys. The index supports key rotation, removal, snapshots, and server-key validity checks. EnvironmentService uses the index during polling, fetching, request validation, and environment removal. Removal clears documents, contexts, identity overrides, and endpoint caches. Configuration may omit environment key pairs. Startup logs a warning when no environments are configured.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟠 High · up to 6eead

The runtime-mutable environment index can currently mishandle colliding keys and can briefly serve data fetched for a replaced environment. These are concrete correctness risks that could expose the wrong environment’s flags or identities, so the PR is not merge-ready until collision handling and replacement validation are fixed.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7f797395-a335-4f14-b32b-be624b0136a6

📥 Commits

Reviewing files that changed from the base of the PR and between 62bb3fc and 6eeada4.

📒 Files selected for processing (8)
  • src/cache/environment.rs
  • src/config/settings.rs
  • src/environments.rs
  • src/lib.rs
  • src/main.rs
  • src/services/environment.rs
  • tests/test_lru_cache.rs
  • tests/test_remove_environment.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment thread src/environments.rs
Comment on lines +103 to +106
for server_key in &keys.server_keys {
by_key.insert(server_key.key.clone(), Arc::clone(&keys));
}
by_key.insert(keys.client_key.clone(), keys);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Reject key collisions before modifying by_key.

EnvironmentKeyPair permits duplicate server keys and client keys that match another indexed key. Line 104 overwrites the existing owner. A later removal of the original environment removes that shared entry, even when it now belongs to the replacement environment.

Require every client key and server key to be unique across the index. Reject an update atomically when it conflicts with another environment.

Comment on lines +111 to +118
/// The poll-loop counterpart: the loop iterates a snapshot, so a
/// removal completing mid-fetch would have its environment's data
/// re-inserted by put_environment and pinned until restart.
async fn discard_environment_if_removed(&self, keys: &EnvironmentKeys) {
if self.environments.resolve(&keys.client_key).is_none() {
self.cache.remove_environment(&keys.client_key).await;
self.clear_endpoint_caches(keys).await;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Reject a poll result when the indexed environment was replaced.

Line 115 checks only that client_key still resolves. If EnvironmentIndex::insert replaces an environment with the same client key during fetch_environment, the old snapshot writes its document at line 68 and this check succeeds. The proxy can then serve data fetched with the replaced server key.

Use the snapshot Arc<EnvironmentKeys> as a generation token. Keep the result only when the current resolved entry is the same instance. Add a concurrent replacement test for this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants